Allow comments as iodata. #721
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
my example app: |
|
I would prefer to keep it as a binary, in case we need to do checks in the future. This is really something we don't want to get wrong and we want to have space to fix it in the future. For example, I am almost sure we should check for null bytes too. There is basically no reason to have either null bytes or */ in a comment. I think appending |
|
null byte terminates the message but postgres requires it to be certain length: |
|
Yup! It should be null byte safe today. But it is also easy for us to make changes in the future, I don't know, maybe use text for some queries, and then completely forget we assumed we are null byte safe here. :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When I wrote the test for the original implementation, I noticed that it couldn't be exploited when there was a
;at the end of the original statement. Postgres returned an error stating,cannot insert multiple commands into a prepared statement.Initially, I abandoned this idea as I didn't want to be the person who introduced SQLI into Postgrex. However, the more I think about it, the more useful it seems to remove the binary requirement for scanning. Without validation, we can use Iodata.I sought advice and even posted a potentially vulnerable app on a security forum. I also conducted a self-scan using SQLMap, and it seems safe. Golang PGX is using this.
The only issue I found was at https://adeadfed.com/posts/postgresql-select-only-rce/, but I don't think it can be used after
;looking at the example. I tried but no luck.Also sqlcommenter specification require to escape the string.